feat: add more db info on span data #3231

armcknight · 2023-08-17T23:33:58Z

📜 Description

Add the persistent store types and URLs to the span data for Core Data tracking.

Also a couple refactors:

fix a copypasta variable name and log statement that were "fetch" but should've been "save"
the fetch tracker method grabs a span inside block scope and then the rest of the method proceeds afterwards; but in save tracking, it's all done in the block scope. bump out the save method's logic to match the fetch method's (less indentation -> more code per line).
rename method to be more descriptive
moved/combined SentryCoreDataTrackerTests.TestNSManagedContext to TestCoreDataStack.managedObjectContext

💡 Motivation and Context

github-actions · 2023-08-17T23:35:10Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- add more db info on span data ([#3231](https://github.com/getsentry/sentry-cocoa/pull/3231))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against bc5e393

armcknight · 2023-08-17T23:34:35Z

Sources/Sentry/SentryCoreDataTracker.m

+    NSMutableArray<NSString *> *systems = [NSMutableArray<NSString *> array];
+    NSMutableArray<NSString *> *names = [NSMutableArray<NSString *> array];
+    [context.persistentStoreCoordinator.persistentStores enumerateObjectsUsingBlock:^(
+        __kindof NSPersistentStore *_Nonnull obj, NSUInteger idx, BOOL *_Nonnull stop) {
+        [systems addObject:obj.type];
+        if (obj.URL != nil) {
+            [names addObject:obj.URL.path];
+        } else {
+            [names addObject:@"(null)"];
+        }
+    }];
+    [span setDataValue:[systems componentsJoinedByString:@";"] forKey:@"db.system"];
+    [span setDataValue:[names componentsJoinedByString:@";"] forKey:@"db.name"];


The new db.name and db.system data.

armcknight · 2023-08-17T23:34:54Z

Sources/Sentry/SentryCoreDataTracker.m

-    __block id<SentrySpan> fetchSpan = nil;
+    __block id<SentrySpan> saveSpan = nil;


Renamed this variable

armcknight · 2023-08-17T23:35:24Z

Sources/Sentry/SentryCoreDataTracker.m

-            if (fetchSpan) {
-                [SentryLog
-                    logWithMessage:[NSString
-                                       stringWithFormat:@"SentryCoreDataTracker automatically "
-                                                        @"started a new span with description: %@, "
-                                                        @"operation: %@",
-                                       fetchSpan.description, SENTRY_COREDATA_FETCH_OPERATION]
-                          andLevel:kSentryLevelDebug];
-
-                [fetchSpan setDataValue:operations forKey:@"operations"];
-            }
+            saveSpan = [span startChildWithOperation:SENTRY_COREDATA_SAVE_OPERATION
+                                         description:[self descriptionForOperations:operations
+                                                                          inContext:context]];
+            saveSpan.origin = SentryTraceOriginAutoDBCoreData;
        }];
+
+        if (saveSpan) {
+            SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically started a new span with "
+                             @"description: %@, operation: %@",
+                saveSpan.description, saveSpan.operation);
+
+            [saveSpan setDataValue:operations forKey:@"operations"];
+        } else {
+            SENTRY_LOG_ERROR(@"managedObjectContext:save:originalImp: saveSpan is nil");
+        }


Moved this out of the useSpan: scope, like the code for fetch operations.

armcknight · 2023-08-17T23:36:04Z

Sources/Sentry/SentryCoreDataTracker.m

-                                       stringWithFormat:@"SentryCoreDataTracker automatically "
-                                                        @"started a new span with description: %@, "
-                                                        @"operation: %@",
-                                       fetchSpan.description, SENTRY_COREDATA_FETCH_OPERATION]


This should've been SENTRY_COREDATA_SAVE_OPERATION, but I did one better and just access SentrySpan.operation for both such logs (here and in fetches)

armcknight · 2023-08-17T23:36:40Z

Sources/Sentry/SentryCoreDataTracker.m

- (void)mainThreadExtraInfo:(SentrySpan *)span
+- (void)addExtraInfoToSpan:(SentrySpan *)span withContext:(NSManagedObjectContext *)context


Renamed this method as it's doing more than just main thread stuff now.

armcknight · 2023-08-17T23:37:21Z

Sources/Sentry/SentryCoreDataTracker.m

-        [SentryLog
-            logWithMessage:[NSString stringWithFormat:
-                                         @"SentryCoreDataTracker automatically "
-                                         @"started a new span with description: %@, operation: %@",
-                                     fetchSpan.description, SENTRY_COREDATA_FETCH_OPERATION]
-                  andLevel:kSentryLevelDebug];
+        SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically started a new span with "
+                         @"description: %@, operation: %@",
+            fetchSpan.description, fetchSpan.operation);


Converted all the logs to the new macros. Also here's the second place I changed interpolating the operation constant for SentrySpan.operation.

Thank you for doing this 👍

github-actions · 2023-08-17T23:46:05Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1245.02 ms	1248.88 ms	3.86 ms
Size	22.84 KiB	404.25 KiB	381.41 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`c2ec420`	1256.27 ms	1269.24 ms	12.97 ms
`e24290f`	1239.94 ms	1248.28 ms	8.34 ms
`28333b6`	1186.29 ms	1225.18 ms	38.89 ms
`a176fc4`	1239.78 ms	1258.98 ms	19.20 ms
`794f87f`	1225.78 ms	1243.46 ms	17.68 ms
`3cba0e8`	1250.86 ms	1258.39 ms	7.53 ms
`7cd187e`	1196.51 ms	1226.04 ms	29.53 ms
`9e96fd6`	1207.20 ms	1229.40 ms	22.20 ms
`0776564`	1224.35 ms	1247.26 ms	22.91 ms
`25e65a5`	1249.08 ms	1277.78 ms	28.70 ms

App size

Revision	Plain	With Sentry	Diff
`c2ec420`	20.76 KiB	401.65 KiB	380.89 KiB
`e24290f`	22.84 KiB	403.51 KiB	380.66 KiB
`28333b6`	20.76 KiB	424.69 KiB	403.93 KiB
`a176fc4`	22.84 KiB	403.24 KiB	380.39 KiB
`794f87f`	20.76 KiB	401.37 KiB	380.61 KiB
`3cba0e8`	22.84 KiB	403.19 KiB	380.34 KiB
`7cd187e`	20.76 KiB	401.66 KiB	380.90 KiB
`9e96fd6`	20.76 KiB	425.80 KiB	405.04 KiB
`0776564`	20.76 KiB	399.19 KiB	378.43 KiB
`25e65a5`	22.85 KiB	403.19 KiB	380.34 KiB

Previous results on branch: armcknight/feat/3212-db-attributes-on-span-data

Startup times

Revision	Plain	With Sentry	Diff
`5c85b82`	1213.31 ms	1228.29 ms	14.98 ms

App size

Revision	Plain	With Sentry	Diff
`5c85b82`	22.84 KiB	404.25 KiB	381.41 KiB

codecov · 2023-08-17T23:48:21Z

Codecov Report

Merging #3231 (8251982) into main (91d2d63) will increase coverage by 0.055%.
The diff coverage is 89.830%.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #3231       +/-   ##
=============================================
+ Coverage   89.153%   89.209%   +0.055%     
=============================================
  Files          502       502               
  Lines        54213     54225       +12     
  Branches     19457     19469       +12     
=============================================
+ Hits         48333     48374       +41     
+ Misses        5014      4990       -24     
+ Partials       866       861        -5

Files Changed	Coverage Δ
...tions/Performance/CoreData/TestCoreDataStack.swift	`82.653% <77.777%> (-2.874%)`	⬇️
Sources/Sentry/SentryCoreDataTracker.m	`94.482% <82.857%> (-1.201%)`	⬇️
...rformance/CoreData/SentryCoreDataTrackerTest.swift	`94.771% <100.000%> (+3.453%)`	⬆️

... and 11 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91d2d63...8251982. Read the comment docs.

armcknight · 2023-08-18T00:15:56Z

Tests/SentryTests/Integrations/Performance/CoreData/SentryCoreDataTrackerTest.swift

+        XCTAssertEqual(try XCTUnwrap(dbSpan.data["db.system"] as? String), "SQLite")
+        XCTAssert(try XCTUnwrap(dbSpan.data["db.name"] as? NSString).contains(TestCoreDataStack.databaseFilename))


Assertions for the new properties

l: maybe re-add the assert for origin and read_count (for read operations)

good catch, i removed those by accident

armcknight · 2023-08-18T00:17:25Z

Tests/SentryTests/Integrations/Performance/CoreData/TestCoreDataStack.swift

+    override var hasChanges: Bool {
+        return ((inserted?.count ?? 0) > 0) ||
+        ((deleted?.count ?? 0) > 0) ||
+        ((updated?.count ?? 0) > 0) || super.hasChanges


Had to add || super.hasChanges for SentryCoreDataTrackerTests.test_Save() to work again, because it doesn't rely on the subclass properties.

brustolin · 2023-08-18T07:05:03Z

Sources/Sentry/SentryCoreDataTracker.m

+        if (obj.URL != nil) {
+            [names addObject:obj.URL.path];
+        } else {
+            [names addObject:@"(null)"];


h: I believe we need a better option to fallback. This is not user friendly and I'm foreseeing complains.

I'm open to any alternatives. Nobody has proposed any so far.

I would also be interested to know the cases where the URL is nil. I expected it to be nil for in memory stores, but even that has a value like memory://563428497. It is probably nil if there is no persistent store/coordinator at all, but then I don't know what the complaint would be. In that case I question if we should even be reporting anything at all, but that kind of refactor is out of scope for this issue.

I'm open to any alternatives.

<App Name>:coredata anything really other than (null)

We already gather the equivalent of that in the type.

In this case, we should send some kind of sentinel value so the frontend knows to render a special state equivalent to "there was no database". This seems as good as any.

If I were a customer I would rather know that this value was not available, vs having it covered up by a default value like what you're proposing. It could help find an upstream error.

I agree with @armcknight here - in case this occurs a lot and provides not enough context we can always re-iterate.

kahest

LGTM! Thanks for the opportunistic refactoring as well 💯

kahest · 2023-08-29T15:46:32Z

Sources/Sentry/SentryCoreDataTracker.m

-        [SentryLog
-            logWithMessage:[NSString stringWithFormat:
-                                         @"SentryCoreDataTracker automatically "
-                                         @"started a new span with description: %@, operation: %@",
-                                     fetchSpan.description, SENTRY_COREDATA_FETCH_OPERATION]
-                  andLevel:kSentryLevelDebug];
+        SENTRY_LOG_DEBUG(@"SentryCoreDataTracker automatically started a new span with "
+                         @"description: %@, operation: %@",
+            fetchSpan.description, fetchSpan.operation);


Thank you for doing this 👍

kahest · 2023-08-29T15:52:30Z

Tests/SentryTests/Integrations/Performance/CoreData/SentryCoreDataTrackerTest.swift

+        XCTAssertEqual(try XCTUnwrap(dbSpan.data["db.system"] as? String), "SQLite")
+        XCTAssert(try XCTUnwrap(dbSpan.data["db.name"] as? NSString).contains(TestCoreDataStack.databaseFilename))


l: maybe re-add the assert for origin and read_count (for read operations)

…db-attributes-on-span-data

armcknight requested review from philipphofmann and brustolin as code owners August 17, 2023 23:33

armcknight commented Aug 17, 2023

View reviewed changes

armcknight commented Aug 18, 2023

View reviewed changes

brustolin reviewed Aug 18, 2023

View reviewed changes

kahest approved these changes Aug 29, 2023

View reviewed changes

armcknight added 5 commits August 29, 2023 14:21

feat: add more db info on span data

edfef05

changelog

43fb9d0

tests

87fed6c

fix test

1f31a08

wip adding back accidentally removed assertions

9fd258d

armcknight force-pushed the armcknight/feat/3212-db-attributes-on-span-data branch from e902924 to 9fd258d Compare August 29, 2023 18:36

armcknight added 3 commits August 29, 2023 14:48

Merge remote-tracking branch 'origin/main' into armcknight/feat/3212-…

5bf3225

…db-attributes-on-span-data

put assert back in right spot

8251982

fix changelog

bc5e393

armcknight merged commit 87e0836 into main Aug 29, 2023
40 of 43 checks passed

armcknight deleted the armcknight/feat/3212-db-attributes-on-span-data branch August 29, 2023 19:33

kahest linked an issue Aug 30, 2023 that may be closed by this pull request

[Starfish] Add db attributes to database span's span data #3212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add more db info on span data #3231

feat: add more db info on span data #3231

armcknight commented Aug 17, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

armcknight Aug 17, 2023

armcknight Aug 17, 2023

armcknight Aug 17, 2023

armcknight Aug 17, 2023

armcknight Aug 17, 2023

armcknight Aug 17, 2023

kahest Aug 29, 2023

github-actions bot commented Aug 17, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: armcknight/feat/3212-db-attributes-on-span-data

Startup times

App size

codecov bot commented Aug 17, 2023 •

edited

Loading

armcknight Aug 18, 2023

kahest Aug 29, 2023

armcknight Aug 29, 2023

armcknight Aug 18, 2023

brustolin Aug 18, 2023

armcknight Aug 18, 2023

armcknight Aug 18, 2023 •

edited

Loading

brustolin Aug 21, 2023

armcknight Aug 21, 2023 •

edited

Loading

kahest Aug 29, 2023 •

edited

Loading

kahest left a comment

kahest Aug 29, 2023

kahest Aug 29, 2023

		__block id<SentrySpan> fetchSpan = nil;
		__block id<SentrySpan> saveSpan = nil;

		- (void)mainThreadExtraInfo:(SentrySpan *)span
		- (void)addExtraInfoToSpan:(SentrySpan )span withContext:(NSManagedObjectContext )context

		XCTAssertEqual(try XCTUnwrap(dbSpan.data["db.system"] as? String), "SQLite")
		XCTAssert(try XCTUnwrap(dbSpan.data["db.name"] as? NSString).contains(TestCoreDataStack.databaseFilename))

feat: add more db info on span data #3231

feat: add more db info on span data #3231

Conversation

armcknight commented Aug 17, 2023 • edited Loading

📜 Description

💡 Motivation and Context

github-actions bot commented Aug 17, 2023 • edited Loading

Instructions and example for changelog

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 17, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: armcknight/feat/3212-db-attributes-on-span-data

Startup times

App size

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcknight Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcknight Aug 21, 2023 • edited Loading

Choose a reason for hiding this comment

kahest Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

kahest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armcknight commented Aug 17, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023 •

edited

Loading

codecov bot commented Aug 17, 2023 •

edited

Loading

armcknight Aug 18, 2023 •

edited

Loading

armcknight Aug 21, 2023 •

edited

Loading

kahest Aug 29, 2023 •

edited

Loading